-
Notifications
You must be signed in to change notification settings - Fork 202
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Address missing processor JsonPropertyDescriptions and validations #4837
Address missing processor JsonPropertyDescriptions and validations #4837
Conversation
3582ebd
to
a35679b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great. Thanks for working on this!
@graytaylor0 looks like tests are failing. |
@@ -78,6 +79,10 @@ public AggregateProcessor(final AggregateProcessorConfig aggregateProcessorConfi | |||
this.localMode = aggregateProcessorConfig.getLocalMode(); | |||
|
|||
pluginMetrics.gauge(CURRENT_AGGREGATE_GROUPS, aggregateGroupManager, AggregateGroupManager::getAllGroupsSize); | |||
|
|||
if (aggregateProcessorConfig.getWhenCondition() != null && (!expressionEvaluator.isValidExpressionStatement(aggregateProcessorConfig.getWhenCondition()))) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes sense to me. Are we adding this validation to all processors now to fail fast on invalid expressions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes that's right
@@ -57,7 +57,7 @@ public class CsvProcessorConfig { | |||
private List<String> columnNames; | |||
|
|||
@JsonProperty("csv_when") | |||
@JsonPropertyDescription("Allows you to specify a [conditional expression](https://opensearch.org/docs/latest/data-prepper/pipelines/expression-syntax/), " + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might be wrong but I thought we want to keep this markdown format here so it will be rendered in the documentation website. @chenqi0805
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We aren't rendering the documentation website from this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not today, but we are planning to do that, see this: opensearch-project/documentation-website#7651
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right. We plan to generate the config table from those annotations
Thanks for reviewing. Yeah looks like I missed the GeoIP integration tests I'll fix them |
d5b5d8d
to
1747c31
Compare
1747c31
to
b86fc7f
Compare
Signed-off-by: Taylor Gray <tylgry@amazon.com>
…n, add @Valid annotation for DDB source config models Signed-off-by: Taylor Gray <tylgry@amazon.com>
b86fc7f
to
52aef81
Compare
Signed-off-by: Taylor Gray <tylgry@amazon.com>
Description
Adds JsonPropertyDescriptions to all remaining processors that were missing them, and adds validation on construction of the processors for their
when
condition in the processors that were missing this validationCheck List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.